fix(runtime): drop the runnables manually, and forget them on other threads#493
fix(runtime): drop the runnables manually, and forget them on other threads#493Berrysoft wants to merge 1 commit into
Conversation
Berrysoft
added
bug
There was a problem hiding this comment.
Pull Request Overview
This PR addresses runtime cleanup issues by implementing proper resource management during Runtime shutdown. The changes ensure that runnables (async tasks) are safely dropped in the correct thread context, and implements a mechanism to forget runnables that are woken on other threads after the runtime has been destroyed.
Key changes:
- Added a
Dropimplementation forRuntimethat clears timer runtime before scheduler cleanup - Implemented thread-safety checks to prevent unsafe cross-thread runnable drops
- Added a test case to verify proper cleanup of detached spawned tasks
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| compio-runtime/tests/drop.rs | New test verifying runtime cleanup with detached spawned tasks |
| compio-runtime/src/runtime/time.rs | Added clear() method to TimerRuntime for cleanup |
| compio-runtime/src/runtime/scheduler/mod.rs | Implemented thread-safety check and clear() method for scheduler cleanup |
| compio-runtime/src/runtime/mod.rs | Added Drop implementation to orchestrate cleanup of timer runtime and scheduler |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Some(task_queue) = task_queue.upgrade() { | ||
| task_queue.push(runnable, ¬ify); | ||
| } else if thread_guard.get().is_none() { | ||
| // It's not safe to drop the runnable in another thread. |
There was a problem hiding this comment.
The comment should explain why it's not safe to drop the runnable in another thread, and clarify that the condition thread_guard.get().is_none() means we're on a different thread. Consider expanding: 'If we're on a different thread (thread_guard.get().is_none()) and the task queue has been dropped, we cannot safely drop the runnable because [reason], so we forget it to prevent the drop from running.'
| // It's not safe to drop the runnable in another thread. | |
| // If we're on a different thread (thread_guard.get().is_none()) and the task queue has been dropped, | |
| // we cannot safely drop the runnable because its destructor may access thread-local or queue-related resources | |
| // that are only valid on the original thread. Dropping it here could lead to undefined behavior. | |
| // Therefore, we use std::mem::forget to prevent the destructor from running. |
| pub(crate) fn clear(&self) { | ||
| loop { | ||
| // SAFETY: | ||
| // `Scheduler` is `!Send` and `!Sync`, so this method is only called | ||
| // on `TaskQueue`'s creator thread. | ||
| let tasks = unsafe { self.task_queue.pop() }; | ||
|
|
||
| if let (None, None) = tasks { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The clear() method pops tasks but doesn't explicitly drop them or handle them in any way. While Rust will drop the returned tasks automatically when they go out of scope at the end of each loop iteration, this implicit behavior should be documented to clarify that the purpose is to drop all pending tasks. Consider adding a comment explaining this or explicitly binding and dropping: let _tasks = unsafe { self.task_queue.pop() }; // Drop all pending tasks.
|
I think I’ve figured out how |
|
Great! Go ahead please. |
|
Closed in favor of #494 |
cc: @Paraworker
The timer runtime is cleared before the scheduler, to drop the remaining wakers. All runnables are dropped in the context of current runtime.
The runnable will be forgotten, if the runtime has been dropped, and the waker is waked on another thread.